Fix symbol visibility issues, add test for it #1359

To clarify, the fix is in the first commit. The excerpt from the CI log with an error message from the previous iteration of this branch looks as follows:

+ python3 ./tools/symbol-check.py .libs/libsecp256k1.so
.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
.libs/libsecp256k1.so: failed EXPORTED_SYMBOLS
Error: Process completed with exit code 1.

@fanquake suggested that the test could potentially be simplified for c-i.

Thanks! The tools/symbol-check.py script has been simplified.

447334c include: Avoid visibility("default") on Windows (Tim Ruffing) Pull request description: Fixes #1421. See code comments for rationale. Related meta-bug: #1181. This reminds me that we should move forward with #1359. ACKs for top commit: fanquake: ACK 447334c hebasto: ACK 447334c, tested on Ubuntu 24.04 using the following commands: theuni: ACK 447334c Tree-SHA512: aaa47d88fd1b1f85c3e879a2b288c0eb3beebad0cc89e85f05d0b631f83e58d5a324fb441911970865eaa292f6820d03a1b516d6e8de37a87510e2082acc6e28

theuni

Code review ACK 44a9392. No opinion on the tests/ci.

.github/workflows/ci.yml

tools/symbol-check.py

ci/ci.sh

tools/symbol-check.py

src/precomputed_ecmult.h

tools/symbol-check.py

real-or-random · 2024-10-23T10:53:53Z

src/util_local_visibility.h

I think this is small enough to keep it in util.h. Or is there a reason not to keep it here?

The reason is to facilitate the further attempts to make all headers self-contained.

I guess I'm just missing it. Can you explain how this will help with #1039?

My intention was to avoid bringing the high-level ../include/secp256k1.h header into lower-level code via util.h. However, at this point, util.h is already being included indirectly through other headers.

No opinion here, I defer to @real-or-random for preference.

hebasto · 2024-10-24T10:07:57Z

@real-or-random's comments have been addressed.

fanquake · 2024-10-24T10:18:16Z

tools/symbol-check.py

+
+def check_MACHO_exported_functions(library, expected_exports) -> bool:
+    ok: bool = True
+    macho_lib: lief.MACHO.Binary = library.concrete


What are the calls to .concrete for?

fanquake · 2024-10-24T10:29:39Z

tools/symbol-check.py

+    elif exe_format == lief.Binary.FORMATS.MACHO:
+        success = check_MACHO_exported_functions(library, grep_exported_symbols())
+    else:
+        print(f'{filename}: unknown executable format')


I don't think this code can be reached, because if it is an unknown format, .parse() will have errored (printing Unknown format), and then lief.Binary.FORMATS = library.format will fail:

./tools/symbol-check.py not_a_file Unknown format Traceback (most recent call last): File "./tools/symbol-check.py", line 81, in <module> exe_format: lief.Binary.FORMATS = library.format ^^^^^^^^^^^^^^ AttributeError: 'NoneType' object has no attribute 'format'

Thanks! Reworked.

I don't think this code can be reached,

This will be reachable once LIEF adds support for a new binary format, so I think it was useful to have it.

hebasto · 2024-10-24T11:38:32Z

@fanquake's feedback has been addressed.

real-or-random

This script can still be simplified. Let me suggest a refactored version later.

real-or-random · 2024-11-01T16:54:59Z

tools/symbol-check.py

+
+def check_PE_exported_functions(library, expected_exports) -> bool:
+    ok: bool = True
+    for function in library.exported_functions:


After reading the docs, I believe that .exported_functions is from the abstract Binary class, and [entry.name for entry in library.get_export()] may give the better result for PE because it includes non-function exports? See https://lief.re/doc/latest/formats/pe/python.html#export-entry

The suggested approach returns the same list of symbols on the master branch. Incorporated as a potentially superior one.

real-or-random · 2024-11-01T16:55:00Z

tools/symbol-check.py

+
+def check_MACHO_exported_functions(library, expected_exports) -> bool:
+    ok: bool = True
+    for function in library.exported_functions:


After reading the docs, I believe that .exported_functions is from the abstract Binary class, and .exported_symbols may give a better result for Mach-O because it includes non-function exports?

The suggested approach returns the same list of symbols on the master branch. Incorporated as a potentially superior one.

Hm, just checking: You're saying that this change didn't make a difference, right? So it still finds only exported functions and not exported variables?

(And I have the same question for PE.)

Hm, just checking: You're saying that this change didn't make a difference, right?

Correct.

So it still finds only exported functions and not exported variables?

With the reverted second commit, both variants catch the same local variables:

./tools/symbol-check.py: In .libs/libsecp256k1.dylib: Unexpected exported symbols: {'secp256k1_pre_g', 'secp256k1_pre_g_128', 'secp256k1_ecmult_gen_prec_table'}

(And I have the same question for PE.)

With the reverted second commit, both variants do not catch local variables.

For more details, please refer to:

https://github.com/hebasto/secp256k1/actions/runs/11640943476

https://github.com/hebasto/secp256k1/actions/runs/11640955643

Ok, that means that the LIEF API is a bit unexpected. Or we don't understand it.

For Mach-O, exported_functions is the same as exported_symbols. Both include variables, but I'd expect the former to include only functions.

For PE, there doesn't seem to be a way to find exported variables. By the way, importing variables is rare, but the ellswift example and benchmark import the variable secp256k1_ellswift_xdh_hash_function_bip324, so at least we test this in CI. And thus we can be sure that the variables are actually exported in PE, and it's just LIEF that doesn't show them.

Does this match your understanding? Do you think it's worth asking LIEF about the variables?

Relatedly, on platforms where LIEF gives us both variables and functions, would it make sense also check that all expected symbols are actually exported?

Nevermind, my analysis of variables is wrong. LIEF shows them. I'll revisit this later.

real-or-random · 2024-11-01T17:49:11Z

Here's a refactored version, which is more readable IMHO (haven't tested on Mac or Windows):

#!/usr/bin/env python3
"""Check that a libsecp256k1 shared library exports only expected symbols.

Usage examples:
  - When building with Autotools:
    ./tools/symbol-check.py .libs/libsecp256k1.so
    ./tools/symbol-check.py .libs/libsecp256k1-<V>.dll
    ./tools/symbol-check.py .libs/libsecp256k1.dylib

  - When building with CMake:
    ./tools/symbol-check.py build/lib/libsecp256k1.so
    ./tools/symbol-check.py build/bin/libsecp256k1-<V>.dll
    ./tools/symbol-check.py build/lib/libsecp256k1.dylib"""

import re
import sys
import subprocess

import lief


class UnexpectedExport(RuntimeError):
    pass


def get_exported_exports(library) -> list[str]:
    """Adapter function to get exported symbols based on the library format."""
    if library.format == lief.Binary.FORMATS.ELF:
        return [symbol.name for symbol in library.exported_symbols]
    elif library.format == lief.Binary.FORMATS.PE:
        return [function.name for function in library.exported_functions]
    elif library.format == lief.Binary.FORMATS.MACHO:
        return [function.name[1:] for function in library.exported_functions]
    raise NotImplementedError(f"Unsupported format: {library.format}")


def grep_expected_symbols() -> list[str]:
    """Guess the list of expected exported symbols from the source code."""
    grep_output = subprocess.check_output(
        ["git", "grep", "^SECP256K1_API", "--", "include"], # TODO WHITESPACE
        universal_newlines=True,
        encoding="utf-8"
    )
    lines = grep_output.split("\n")
    pattern = re.compile(r'\bsecp256k1_\w+')
    exported: list[str] = [pattern.findall(line)[-1] for line in lines if line.strip()]
    return exported


def check_symbols(library, expected_exports) -> None:
    """Check that the library exports only the expected symbols."""
    actual_exports = list(get_exported_exports(library))
    unexpected_exports = set(actual_exports) - set(expected_exports)
    if unexpected_exports != set():
        raise UnexpectedExport(f"Unexpected exported symbols: {unexpected_exports}")

def main():
    if len(sys.argv) != 2:
        print(__doc__)
        return 1
    library = lief.parse(sys.argv[1])
    expected_exports = grep_expected_symbols()
    try:
        check_symbols(library, expected_exports)
    except UnexpectedExport as e:
        print(f"{sys.argv[0]}: In {sys.argv[1]}: {e}")
        return 1
    return 0


if __name__ == "__main__":
    sys.exit(main())

hebasto · 2024-11-01T20:33:54Z

Here's a refactored version, which is more readable IMHO (haven't tested on Mac or Windows):

Tested on macOS and on Windows. Incorporated. Thank you!

tools/symbol-check.py

jonasnick · 2024-11-04T13:36:04Z

tools/symbol-check.py

+def grep_expected_symbols() -> list[str]:
+    """Guess the list of expected exported symbols from the source code."""
+    grep_output = subprocess.check_output(
+        ["git", "grep", "^\s*SECP256K1_API", "--", "include"],


This gives me error symbol-check.backup.py:40: SyntaxWarning: invalid escape sequence '\s'. I think it should be

r"^\s*SECP256K1_API"

or

"^\\s*SECP256K1_API"

This change makes the `-fvisibility=hidden` compiler option unnecessary.

Co-authored-by: Tim Ruffing <crypto@timruffing.de>

hebasto · 2024-11-04T17:49:30Z

Rebased and addressed @jonasnick's #1359 (comment).

theuni

Code-review reACK 7ac46cf.

Still no opinion on the .py/ci.

Being able to drop -fvisibility=hidden is a nice improvement (and also a testament to the code structure!)

real-or-random added build assurance labels Jun 26, 2023

hebasto force-pushed the 230626-visibility branch from ee45fa1 to f816729 Compare June 27, 2023 07:45

hebasto force-pushed the 230626-visibility branch from f816729 to d7b1cea Compare July 13, 2023 15:53

hebasto force-pushed the 230626-visibility branch from d7b1cea to be7ccb9 Compare July 13, 2023 16:56

hebasto marked this pull request as draft July 13, 2023 17:36

hebasto force-pushed the 230626-visibility branch from be7ccb9 to 5e37264 Compare July 13, 2023 17:47

hebasto marked this pull request as ready for review July 13, 2023 17:47

hebasto marked this pull request as draft August 31, 2023 14:35

hebasto force-pushed the 230626-visibility branch from 5e37264 to 0203b09 Compare February 12, 2024 18:29

hebasto marked this pull request as ready for review February 12, 2024 18:31

hebasto force-pushed the 230626-visibility branch from 0203b09 to 542ea42 Compare February 13, 2024 17:17

hebasto force-pushed the 230626-visibility branch from 542ea42 to b80241b Compare April 23, 2024 13:01

real-or-random mentioned this pull request Aug 24, 2024

build: 45839th attempt to fix symbol visibility on Windows #1595

Merged

hebasto force-pushed the 230626-visibility branch 5 times, most recently from 7f53ffc to 4ec4220 Compare August 25, 2024 16:18

hebasto force-pushed the 230626-visibility branch from 4ec4220 to b40e4e3 Compare October 16, 2024 11:34

hebasto marked this pull request as draft October 16, 2024 13:07

hebasto force-pushed the 230626-visibility branch 2 times, most recently from f2a6941 to 4f64b9f Compare October 16, 2024 14:01

hebasto marked this pull request as ready for review October 16, 2024 14:34

real-or-random added this to the 0.6.0 milestone Oct 21, 2024

theuni approved these changes Oct 22, 2024

View reviewed changes

real-or-random reviewed Oct 23, 2024

View reviewed changes

hebasto force-pushed the 230626-visibility branch from 44a9392 to 8ee12da Compare October 24, 2024 10:00

fanquake reviewed Oct 24, 2024

View reviewed changes

hebasto force-pushed the 230626-visibility branch from 8ee12da to b80743a Compare October 24, 2024 11:12

real-or-random reviewed Nov 1, 2024

View reviewed changes

hebasto force-pushed the 230626-visibility branch from b80743a to 0c9a046 Compare November 1, 2024 20:27

real-or-random reviewed Nov 1, 2024

View reviewed changes

tools/symbol-check.py Outdated Show resolved Hide resolved

hebasto force-pushed the 230626-visibility branch from 0c9a046 to fb5d25d Compare November 2, 2024 08:06

jonasnick reviewed Nov 4, 2024

View reviewed changes

jonasnick mentioned this pull request Nov 4, 2024

release: prepare for 0.6.0 #1631

Merged

3 tasks

real-or-random modified the milestones: 0.6.0, 0.6.1 Nov 4, 2024

hebasto and others added 2 commits November 4, 2024 17:48

Introduce SECP256K1_LOCAL_VAR macro

9ef06c8

This change makes the `-fvisibility=hidden` compiler option unnecessary.

test: Add tools/symbol-check.py

aad2b79

Co-authored-by: Tim Ruffing <crypto@timruffing.de>

hebasto force-pushed the 230626-visibility branch from fb5d25d to 11d0ec6 Compare November 4, 2024 17:48

hebasto added 2 commits November 4, 2024 18:08

ci: Run tools/symbol-check.py

0b1a0df

build: Drop no longer needed -fvisibility=hidden compiler option

7ac46cf

hebasto force-pushed the 230626-visibility branch from 11d0ec6 to 7ac46cf Compare November 4, 2024 18:08

theuni approved these changes Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symbol visibility issues, add test for it #1359

Fix symbol visibility issues, add test for it #1359

hebasto commented Jun 26, 2023 •

edited

Loading

hebasto commented Jun 27, 2023

hebasto commented Jul 13, 2023

hebasto commented Feb 12, 2024

maflcko commented Feb 13, 2024

hebasto commented Apr 23, 2024

hebasto commented Aug 25, 2024

hebasto commented Oct 16, 2024

theuni left a comment

real-or-random Oct 23, 2024

hebasto Oct 24, 2024

real-or-random Nov 1, 2024

hebasto Nov 2, 2024

theuni Nov 12, 2024

hebasto commented Oct 24, 2024

fanquake Oct 24, 2024

hebasto Oct 24, 2024

fanquake Oct 24, 2024 •

edited

Loading

hebasto Oct 24, 2024

real-or-random Nov 1, 2024

hebasto commented Oct 24, 2024

real-or-random left a comment

real-or-random Nov 1, 2024

hebasto Nov 1, 2024

real-or-random Nov 1, 2024

hebasto Nov 1, 2024

real-or-random Nov 1, 2024

hebasto Nov 2, 2024

real-or-random Nov 4, 2024

real-or-random Nov 4, 2024

real-or-random commented Nov 1, 2024 •

edited

Loading

hebasto commented Nov 1, 2024

jonasnick Nov 4, 2024

hebasto commented Nov 4, 2024

theuni left a comment

Fix symbol visibility issues, add test for it #1359

Are you sure you want to change the base?

Fix symbol visibility issues, add test for it #1359

Conversation

hebasto commented Jun 26, 2023 • edited Loading

hebasto commented Jun 27, 2023

hebasto commented Jul 13, 2023

hebasto commented Feb 12, 2024

maflcko commented Feb 13, 2024

hebasto commented Apr 23, 2024

hebasto commented Aug 25, 2024

hebasto commented Oct 16, 2024

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanquake Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Oct 24, 2024

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random commented Nov 1, 2024 • edited Loading

hebasto commented Nov 1, 2024

Choose a reason for hiding this comment

hebasto commented Nov 4, 2024

theuni left a comment

Choose a reason for hiding this comment

hebasto commented Jun 26, 2023 •

edited

Loading

fanquake Oct 24, 2024 •

edited

Loading

real-or-random commented Nov 1, 2024 •

edited

Loading